-
Notifications
You must be signed in to change notification settings - Fork 259
feat(server): Support A2A protocol (apache#1762) #2656
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: master
Are you sure you want to change the base?
Conversation
- Support JWKS for A2A compliant secure agent authentication - Enable key rotation without restarting the server - Allow agents from different tenants to publish to the same Iggy bus
|
hey! thanks for contribution - we'll check this after the weekend. |
|
|
||
| #[derive(Debug, Deserialize)] | ||
| struct Jwk { | ||
| kty: String, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we make it an enum, you could use strum with all the helpers like here:
#[derive(Clone, Copy, Debug, Default, Display, PartialEq, Eq, Serialize, Deserialize)]
#[strum(serialize_all = "snake_case")]
#[serde(rename_all = "snake_case")]
pub enum McpTransport {
#[default]
#[strum(to_string = "http")]
Http,
#[strum(to_string = "stdio")]
Stdio,
}
Then the match you do later on becomes easier.
| kid: &str, | ||
| ) -> Result<DecodingKey, anyhow::Error> { | ||
| if let Err(e) = self.refresh_keys(issuer, jwks_url).await { | ||
| return Err(anyhow::anyhow!("Failed to refresh keys: {}", e)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please use the existing custom error enums we already have - feel free to add new custom variants if needed or reuse existing ones.
|
|
||
| #[derive(Debug, Clone)] | ||
| pub struct JwksClient { | ||
| cache: Arc<IggyRwLock<HashMap<CacheKey, DecodingKey>>>, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe just use DashMap instead?
| .ok_or_else(|| anyhow::anyhow!("Key not found in cache after refresh")) | ||
| } | ||
|
|
||
| async fn refresh_keys(&self, issuer: &str, jwks_url: &str) -> Result<(), anyhow::Error> { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same here regarding all the errors, don't use anyhow, please fix in all places returning errors.
|
|
||
| for key in jwks.keys { | ||
| if let Some(kid) = key.kid { | ||
| let decoding_key: DecodingKey = match key.kty.as_str() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Here you could match using the previously added enum, also makes sense to make the string lowercase first.
| if let Some(config) = self.trusted_issuer.get(&insecure.claims.iss) { | ||
| debug!("Found trusted issuer config: {}", config.issuer); | ||
| if let Some(kid_str) = kid.as_deref() { | ||
| if let Some(decoding_key) = self |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please try to avoid multiple levels of if statements, better to return/break quickly whenever possible with let Some() or let Ok() patterns.
|
Thank you for the contribution, made a few comments here and there :) Is that all required to fully support A2A, as you wrote that I'd close #1762 which is the full integration? Also, is there a way to do the proper integration/e2e testing like e..g for existing MCP runtime to ensure it works well with A2A as the full transport? |
| rustls-pemfile = { workspace = true } | ||
| send_wrapper = { workspace = true } | ||
| serde = { workspace = true } | ||
| serde_json.workspace = true |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Stick to serde_json = { workspace = true } etc. everywhere
|
|
||
| Ok(()) | ||
| } | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please extend this file with unit tests matching the existing conventions to ensure that at least the logic JwksClientlikerefresh_keys` or so is correct.
|
Thanks for the review! All comments are clear and I will address every point as suggested. |
Which issue does this PR close?
Closes #1762
Rationale
A2A protocol requires JWKS support to enable secure agent authentication with multiple identity providers. This change allows agents from different tenants to authenticate using their own public keys, and supports key rotation without requiring server restarts.
What changed?
Added JWKS support for secure agent-to-agent authentication. The implementation includes a JwksClient that fetches and caches public keys from JWKS endpoints, integrated JWKS into JwtManager for multi-tenant agent authentication, and updated HTTP middleware to support asynchronous JWT decoding. Also added TrustedIssuerConfig to support configuring multiple trusted issuers.
Local Execution
AI Usage
debug!to help me find bugs。cargo check --package serverandcargo build --package server.